Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Fuzz.filterMap #220

Merged
merged 4 commits into from
Jan 20, 2024

Conversation

mbaechler
Copy link
Contributor

@mbaechler mbaechler commented Aug 17, 2023

I needed this function on a project I'm working on so I figured it could be interesting to contribute it back upstream.

edit: By the way, I couldn't run the test locally, I expect to CI to help me about that.

@Janiczek
Copy link
Collaborator

Hey there! Thanks for the contribution.

Can you please provide more info about your usecase? Just so that I can get my head around when this code would be useful.

Re tests, you can try ./tests/run-tests.sh. The setup is a bit special as this is elm-explorations package with kernel JS modules.

The code itself looks good, I'd just

  • add the new function to the docs: on line 65,
@docs constant, invalid, filter, filterMap
  • use a more realistic example in the doc for the filterMap function. Right now it's just the filter doc with Just a instead of True and Nothing instead of False. I don't yet know what example that should be though.
  • Add a test for the relationship between filter and filterMap: something along the lines of
fuzzer |> filterMap f |> map ((/=) Nothing)
===
fuzzer |> filter (f >> (/=) Nothing)

@lue-bird
Copy link

Examples of cases when you want filterMap are the same as with filter, only that you get a more narrow type when it succeeds, like

module UnicodeNonLetter exposing (UnicodeNonLetter, fromChar, fuzz)

type UnicodeNonLetter = UnicodeNonLetter Char

fromChar : Char -> Maybe UnicodeNonLetter
fromChar c =
    if (c |> Unicode.isLower |> not) && (c |> Unicode.isUpper |> not) then
        UnicodeNonLetter |> Just

    else
        Nothing


fuzz : Fuzzer UnicodeNonLetter
fuzz =
    Fuzz.char |> Fuzz.filterMap fromChar

@Janiczek
Copy link
Collaborator

Sounds lovely - that would be a nice example to have in the docs 👍

@mbaechler
Copy link
Contributor Author

Hi @Janiczek , sorry for the delay but thanks for your review.

use a more realistic example in the doc for the filterMap function. Right now it's just the filter doc with Just a instead of True and Nothing instead of False. I don't yet know what example that should be though.

Is it ok if I borrow the example from @lue-bird ?

Add a test for the relationship between filter and filterMap

Could you provide me some guidance about that?

@Janiczek
Copy link
Collaborator

Is it ok if I borrow the example from @lue-bird ?

Sure thing!

Could you provide me some guidance about that?

Actually, ignore this requirement. I wasn't fully switched into the "Fuzzers are functions" train of thought when writing that... I was thinking in Lists. I don't think we have an easy way to test equivalence of fuzzers right now. There might be some in the future but let's skip this for now.

@mbaechler
Copy link
Contributor Author

Hi @Janiczek ,

I finally found some time to update my PR, it should be good now.

Let me know if there are still things to fix.

@Janiczek
Copy link
Collaborator

Hey @mbaechler, the CI tests failed because of a forgotten stale version in tests/elm.json. I've fixed it here: 81b15e2

I'm gonna pull master into your branch and try the tests again.

@Janiczek
Copy link
Collaborator

Thanks for your work on this!

@Janiczek Janiczek merged commit 5122599 into elm-explorations:master Jan 20, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants